Skip to content

fix(macOS): keep the GUI responsive when many files are added to a sync folder#10370

Open
ftapps wants to merge 5 commits into
nextcloud:masterfrom
ftapps:fix/macos-gui-freeze-many-files
Open

fix(macOS): keep the GUI responsive when many files are added to a sync folder#10370
ftapps wants to merge 5 commits into
nextcloud:masterfrom
ftapps:fix/macos-gui-freeze-many-files

Conversation

@ftapps

@ftapps ftapps commented Jul 15, 2026

Copy link
Copy Markdown

Problem

On macOS the client's window freezes for as long as it takes to process a large
batch of files, in two situations users hit routinely:

  • many files are added at once to a watched sync folder;
  • a new account is set up against a server that already holds a large tree.

The sync itself is fine — it is the GUI that stops responding until the initial
processing is over.

Cause

FolderWatcherPrivate::startWatching() scheduled the FSEvents stream on
dispatch_get_main_queue(), so the whole notification pipeline ran on the GUI
thread, unlike the Windows backend which uses its own WatcherThread. The work
done there is unbounded in the number of files:

  • addCoalescedPaths() walked every reported directory's whole subtree with
    QDirIterator and deduplicated it with a linear QStringList::contains();
  • notifyAll() — reached on kFSEventStreamEventFlagMustScanSubDirs or a
    kernel drop, which is exactly what a mass copy triggers — enumerated the
    entire sync folder and reported every path individually;
  • each reported path then costs a journal lookup and several stats in
    Folder::slotWatchedPathChanged().

The setup case has a separate, cross-platform cause: SyncEngine::slotItemDiscovered()
kept _syncItems sorted with a lower_bound + QVector::insert per item, which
memmoves the tail every time and costs O(n²) overall, all of it on the GUI thread
before the first byte is transferred.

Changes

Four self-contained commits:

  1. fix(macOS): handle file system events off the GUI thread — private serial
    dispatch queue, batches handed to the watcher's thread with a queued
    invocation; subtree expansion bounded and deduplicated with a QSet;
    notifyAll() asks for a full local discovery instead of walking the tree.
  2. fix: keep a full local discovery request across a running sync — a request
    arriving mid-sync was silently dropped, because the running sync restarts
    _timeSinceLastFullLocalDiscovery when it finishes. This one is pre-existing
    and not macOS-specific, but the change above depends on the request being
    honoured.
  3. perf: sort discovered sync items once instead of inserting in order — O(n²) → O(n log n).
  4. perf: cache the selective sync list in the journal — it was queried once per added file.

Testing

Built on macOS (Qt 6) and run as the daily driver against a real server, on a
sync folder of ~84 GB.

  • FolderWatcherTest, LocalDiscoveryTest, SyncJournalDBTest, SyncMoveTest,
    RemoteDiscoveryTest, SyncConflictTest, SyncFileStatusTrackerTest,
    AllFilesDeletedTest, BlacklistTest, SyncDeleteTest all pass. A debug
    build covers commit 3 through the Q_ASSERT(std::is_sorted(...)) in
    finishSync() and OwncloudPropagator::start().
  • testMove3LevelDirWithFile in particular still passes: FSEvents does not
    report the items below a renamed directory, so the expansion is still needed --
    it is bounded, not removed.
  • Manual: copied a folder of ~101k files / 13 GB into the sync folder. The window
    stayed responsive throughout, where it used to freeze. sample(1) on the main
    thread during discovery shows it parked in nextEventMatchingMask with no
    FolderWatcher/ProcessDirectoryJob frames on it.
  • By accident this also got an A/B run: a stale login item relaunched the
    released 33.0.7 build alongside the patched one, against the same account and
    folder. Activity Monitor showed the released build as "Not Responding" while
    the patched one kept its main thread ~80% idle under the same load.

Notes for reviewers

  • addCoalescedPaths() deliberately bounds only its own expansion, not the
    incoming batch: those are events FSEvents actually reported, they are capped by
    the kernel's own buffer, and reporting them drives things a full local
    discovery cannot redo, such as releasing office file locks.
  • changesLost() reports no path on purpose. The watched root reaches the local
    discovery tracker as an empty relative path, and {""} matches only the root
    itself, not the tree below it — so it would look like a fix while quietly
    discovering nothing.
  • The sync engine still runs on the GUI thread; this does not change that. It
    removes the unbounded single blocks, so the remaining work yields to the event
    loop between batches.

ftapps added 4 commits July 15, 2026 15:55
The FSEvents stream was scheduled on the main dispatch queue, so every event
batch was handled on the GUI thread. Dropping a large tree into the sync folder
made that work unbounded: addCoalescedPaths() walked each reported directory's
whole subtree with QDirIterator and deduplicated it with a linear
QStringList::contains(), and on kFSEventStreamEventFlagMustScanSubDirs or a
kernel drop -- exactly what a mass copy triggers -- notifyAll() enumerated the
entire sync folder and reported every path one by one. Each reported path then
costs a journal lookup and several stats in Folder::slotWatchedPathChanged(),
so the window stayed frozen until the whole batch was processed.

Deliver events on a private serial queue instead, as the Windows backend
already does with its own WatcherThread, and hand each batch over to the
FolderWatcher's thread with a queued invocation. Deduplicate the expansion with
a QSet rather than a linear scan, and stop expanding past maxCoalescedPaths:
nothing is lost by stopping, because the directory being expanded was itself
reported and SyncEngine::shouldDiscoverLocally() descends into a touched
directory's entire subtree.

notifyAll() no longer walks the tree either: a full local discovery re-reads it
anyway, so it just emits lostChanges(). It reports no path deliberately -- the
watched root would reach the local discovery tracker as an empty relative path,
which matches only the root itself and not the tree below it.

Signed-off-by: Felipe Tumonis <ftumonis@gmail.com>
slotNextSyncFullLocalDiscovery() is documented to ensure the next sync performs
a full local discovery, but it only invalidated _timeSinceLastFullLocalDiscovery.
A sync that was already running restarts that timer when it finishes, so a
request arriving mid-sync -- the common case, since the watcher emits
lostChanges() precisely while changes are pouring in -- was silently dropped and
the next sync read from the database instead. The missed changes then waited for
the periodic full local discovery, an hour by default, or forever where that
interval is configured negative.

Track whether a request arrived after the running sync fixed its discovery
style, and only mark a full local discovery as done when none did.

While here, check the selective sync list in warnOnNewExcludedItem() before
stat()ing the file: the function runs once per added file and the list is empty
unless the user configured selective sync.

Signed-off-by: Felipe Tumonis <ftumonis@gmail.com>
slotItemDiscovered() kept _syncItems sorted by inserting each item at its
lower_bound. Inserting into the middle of a contiguous vector memmoves the tail,
so discovering n items costs O(n^2): for a first sync of a large tree that is
billions of shared-pointer moves on the GUI thread, all of it before the first
file is transferred.

Nothing reads _syncItems before discovery ends -- the first consumer is
handleMassDeletion(), from slotDiscoveryFinished() -- so append and sort once
there. Stable, so that items comparing equal keep the order they were
discovered in.

Signed-off-by: Felipe Tumonis <ftumonis@gmail.com>
getSelectiveSyncList() runs a full query and materializes the whole list on
every call, and Folder::warnOnNewExcludedItem() calls it once per file added to
the sync folder. The list only changes when the user edits the selective sync
configuration, so keep it in memory and drop it in setSelectiveSyncList(), the
only writer of the table, and on close().

Signed-off-by: Felipe Tumonis <ftumonis@gmail.com>
@ftapps ftapps force-pushed the fix/macos-gui-freeze-many-files branch from 1d576a9 to 0de37dc Compare July 15, 2026 18:57

@claucambra claucambra left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ftapps how did you reproduce the issue before the fix and verify it worked after the fact? Would like to test on my own system

If possible it would also be good to formalise this into a proper test so we can guard against regressions on this :)

The macOS folder watcher stops expanding an FSEvents batch at its coalescing
limit and reports only the directory it was expanding, relying on the sync
engine discovering that directory's whole subtree from the single entry. If
SyncEngine::shouldDiscoverLocally() ever stopped descending into a reported
directory, the watcher would silently miss every file below it -- a data-loss
regression with no visible error.

Add a TestLocalDiscovery case that pins the guarantee: with only "A" in the
local discovery set, entries arbitrarily deep under it must still be discovered,
while unrelated siblings must not. Verified to fail if the subtree branch of
shouldDiscoverLocally() is broken.

Signed-off-by: Felipe Tumonis <ftumonis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants